Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Custom metric-profile list from flag #111

Merged
merged 2 commits into from
Oct 3, 2024

Conversation

rsevilla87
Copy link
Member

Type of change

  • Refactor
  • New feature
  • Bug fix
  • Optimization
  • Documentation Update

Description

Small implementation of the --metrics-profile flag

Related Tickets & Documents

Checklist before requesting a review

  • I have performed a self-review of my code.
  • If it is a core feature, I have added thorough tests.

@rsevilla87 rsevilla87 requested review from a team as code owners September 25, 2024 13:21
@rsevilla87 rsevilla87 changed the title Issue 110 Custom metric-profile from flag Sep 25, 2024
@rsevilla87 rsevilla87 changed the title Custom metric-profile from flag Custom metric-profile list from flag Sep 25, 2024
@rsevilla87 rsevilla87 changed the title Custom metric-profile list from flag [WIP] Custom metric-profile list from flag Sep 25, 2024
@rsevilla87 rsevilla87 requested a review from a team as a code owner September 26, 2024 09:02
@rsevilla87 rsevilla87 force-pushed the issue-110 branch 6 times, most recently from 23ef16c to a07d303 Compare September 27, 2024 11:36
@davdhacs
Copy link

davdhacs commented Oct 1, 2024

This looks like it it exactly what I need. Thank you!

@rsevilla87 rsevilla87 changed the title [WIP] Custom metric-profile list from flag Custom metric-profile list from flag Oct 2, 2024
@rsevilla87
Copy link
Member Author

This looks like it it exactly what I need. Thank you!

Great, have you tested it?

Copy link
Contributor

@jtaleric jtaleric left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm - @davdhacs can you confirm you have tested and this fits the needs for the ACS team?

profileType, _ := cmd.Root().PersistentFlags().GetString("profile-type")
switch ProfileType(profileType) {
case Reporting:
metricsProfiles = []string{"metrics-report.yml"}
case Regular:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to remove this from the help message in ocp.go as well

Copy link
Member Author

@rsevilla87 rsevilla87 Oct 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to touch any of the current functionality, for now, in a follow-up PR my idea is to deprecate the profile-type flag too

Copy link
Contributor

@vishnuchalla vishnuchalla Oct 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without regular case being handled here, I did not see a point in having it in the help description as its is dead code and might be misleading for users.
Can you provide more details on deprecating profile-type flag please? It might break our CI.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it will break it, for sure, for that reason I prefer to address that separately

Copy link
Contributor

@vishnuchalla vishnuchalla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@rsevilla87 rsevilla87 merged commit fd4c2d7 into kube-burner:main Oct 3, 2024
4 checks passed
@davdhacs
Copy link

davdhacs commented Oct 3, 2024

lgtm - @davdhacs can you confirm you have tested and this fits the needs for the ACS team?

+1 it works in my use case in openshift-ci: openshift/release#57412

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RFE] Set custom metric-profiles from flag
4 participants